Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Unable to view details of product after scanning #1726

Merged
merged 7 commits into from
May 3, 2022
Merged

fix: Unable to view details of product after scanning #1726

merged 7 commits into from
May 3, 2022
+9 −9

Conversation

bhattabhi013
Copy link
Contributor

What

Fixes Unable to view details of the product after scanning.

Screenshot

image

Fixes bug(s)

Part of

@bhattabhi013 bhattabhi013 requested a review from a team as a code owner April 30, 2022 19:23
@bhattabhi013 bhattabhi013 changed the title Issue#1725 fix: Unable to view details of product after scanning Apr 30, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2022 •
edited
Loading

Codecov Report

Merging #1726 (0c18e5e) into develop (2ea0da3) will decrease coverage by 0.59%.
The diff coverage is 0.67%.

@@            Coverage Diff             @@
##           develop   #1726      +/-   ##
==========================================
- Coverage     8.86%   8.27%   -0.60%     
==========================================
  Files          161     165       +4     
  Lines         6623    7145     +522     
==========================================
+ Hits           587     591       +4     
- Misses        6036    6554     +518
Impacted Files Coverage Δ
...h_app/lib/cards/category_cards/abstract_cache.dart 0.00% <0.00%> (ø)
...p/lib/cards/category_cards/asset_cache_helper.dart 0.00% <0.00%> (ø)
...p/lib/cards/category_cards/raster_async_asset.dart 0.00% <0.00%> (ø)
...oth_app/lib/cards/category_cards/raster_cache.dart 0.00% <0.00%> (ø)
..._app/lib/cards/category_cards/svg_async_asset.dart 0.00% <0.00%> (ø)
...smooth_app/lib/cards/category_cards/svg_cache.dart 0.00% <0.00%> (ø)
...t_cards/knowledge_panels/knowledge_panel_card.dart 0.00% <0.00%> (ø)
...t_cards/knowledge_panels/knowledge_panel_page.dart 0.00% <0.00%> (ø)
...knowledge_panels/knowledge_panel_summary_card.dart 0.00% <ø> (ø)
...s/knowledge_panels/knowledge_panel_table_card.dart 0.00% <0.00%> (ø)
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ee733a...0c18e5e. Read the comment docs.

monsieurtanuki
monsieurtanuki reviewed Apr 30, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no time to double-check now, and I'll have no time tomorrow (Sunday May 1st).
OK it looks fair to add ContinuousScanModel in main.dart, but I would be VERY surprised if you don't impact ALL the other places in the code that called/created an instance of ContinuousScanModel. It's like a singleton. But if it's also created somewhere else, that's a call for confusion and bugs.

@teolemon teolemon linked an issue May 1, 2022 that may be closed by this pull request
Unable to view details of product after scanning. #1725
Closed
@bhattabhi013
Copy link
Contributor Author

Hi @monsieurtanuki,
I've moved the ContinuousScanModel to main.dart and added if (isRemovable && !isSelectable) to product_title_card.dart to remove cross when SummaryCard is in full screen.

SummaryCard in full screen(cross not required)
image

SummaryCard not in full screen(cross required to remove from carousel)
image

I also checked the impact at onboarding which caused #1720 and everything works fine there.

monsieurtanuki
monsieurtanuki requested changes May 1, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bhattabhi013!
I have no strong opinion about when to put the cross, except that we should check first if the product is in the scan carousel if the idea is to put a button to remove the product from the carousel.
But, my main concern is that we should call only once in the whole app await ContinuousScanModel().load(localDatabase). Please fix that first.

@bhattabhi013
Copy link
Contributor Author

Hi @monsieurtanuki,
I apologize as I'm unable to understand what you are trying to convey but I understood that there should be only one call of await ContinuousScanModel().load(localDatabase) and I can confirm that it is called only once i.e. in scan_page. Please let me know if I'm missing something.

@M123-dev
Copy link
Member

M123-dev commented May 2, 2022

I think what he means it that we now have two Providers of type ContinuousScanModel but only one should be in the whole widget tree

@monsieurtanuki
Copy link
Contributor

@bhattabhi013 ContinuousScanModel() must be called only once in the app, in main.dart. And the value can be retrieved in the whole app through a provider.

monsieurtanuki
monsieurtanuki reviewed May 3, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bhattabhi013, it looks more like that indeed!
Still, I think we could load the model at init time too, right?
Please have a look at my comments and suggestions.

packages/smooth_app/lib/helpers/picture_capture_helper.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/main.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/scan/scan_page.dart Outdated Show resolved Hide resolved
monsieurtanuki
monsieurtanuki approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bhattabhi013!
I just have minor suggestions; feel free to ignore them.

packages/smooth_app/lib/pages/scan/scan_page.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/main.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/main.dart Outdated Show resolved Hide resolved
monsieurtanuki
monsieurtanuki approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bhattabhi013 Let's try it that way!
Possibly, your changes may have a bad impact on the screenshot generation, but that's another story!

@monsieurtanuki monsieurtanuki merged commit f11488e into openfoodfacts:develop May 3, 2022
@github-actions github-actions bot mentioned this pull request May 3, 2022
Closed
@bhattabhi013
Copy link
Contributor Author

Thanks, @monsieurtanuki,
I'm really thankful for all your support and guidance during this PR, I was able to get a better understanding of the provider and app code.
On a side note, I feel this may not impact the screenshot generation because we are using 2 different init's in main.dart .

@monsieurtanuki
Copy link
Contributor

On a side note, I feel this may not impact the screenshot generation because we are using 2 different init's in main.dart

I don't think it's appropriate to use logic regarding screenshot generation. Just painful experiments do the trick. Believe me!
Actually we're using 2 inits just because it seemed to work that way. Don't ask me why.

@github-actions github-actions bot mentioned this pull request Aug 3, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees

@bhattabhi013 bhattabhi013

Labels
🎯 P0
Projects
🤳🥫 The Open Food Facts mobile app (Android & iOS)
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to view details of product after scanning.
4 participants
@bhattabhi013 @codecov-commenter @M123-dev @monsieurtanuki